chore(consensus-types): Make slashings use math.Gwei #2038
Conversation
WalkthroughThe changes in this pull request primarily focus on updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abi87
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2038 +/- ##
=======================================
Coverage 22.13% 22.13%
=======================================
Files 357 357
Lines 16002 16002
Branches 12 12
=======================================
Hits 3542 3542
Misses 12313 12313
Partials 147 147
|
| @@ -69,7 +69,7 @@ type BeaconState[ | |||
| NextWithdrawalValidatorIndex math.ValidatorIndex | |||
|
|
|||
| // Slashing | |||
| Slashings []uint64 | |||
| Slashings []math.Gwei | |||
There was a problem hiding this comment.
this is the main point of the PR. Make Slashing and TotalSlashing share the same type
| @@ -378,7 +378,7 @@ func (st *BeaconState[ | |||
| } | |||
| subIndx = hh.Index() | |||
| for _, i := range st.Slashings { | |||
| hh.AppendUint64(i) | |||
| hh.AppendUint64(uint64(i)) | |||
There was a problem hiding this comment.
q: should I use i.Unwrap() here instead? It seems the principled way, but it makes a copy, while casting should not. We do use the uint64 cast for TotalSlashing as well (line 392)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
mod/storage/pkg/beacondb/slashing.go (1)
Line range hint
32-44: LGTM! Consider pre-allocating the slashings slice for better performance.The changes to the
GetSlashingsmethod align well with the PR objective of usingmath.Gweifor slashings consistently. The conversion fromuint64tomath.Gweiis implemented correctly.To potentially improve performance, consider pre-allocating the
slashingsslice if you have an estimate of the number of slashings. This can reduce memory allocations and copies. For example:-var slashings []math.Gwei +slashings := make([]math.Gwei, 0, estimatedSlashingsCount)Replace
estimatedSlashingsCountwith an appropriate estimate or a constant value based on your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- mod/consensus-types/pkg/types/state.go (3 hunks)
- mod/consensus-types/pkg/types/state_test.go (1 hunks)
- mod/node-api/handlers/proof/merkle/mock/beacon_state.go (1 hunks)
- mod/node-core/pkg/components/interfaces.go (2 hunks)
- mod/state-transition/pkg/core/state/interfaces.go (1 hunks)
- mod/state-transition/pkg/core/state/types.go (1 hunks)
- mod/storage/pkg/beacondb/slashing.go (2 hunks)
🔇 Additional comments (13)
mod/storage/pkg/beacondb/slashing.go (2)
Line range hint
50-105: Excellent consistency across slashing-related methods.The existing methods (
GetSlashingAtIndex,SetSlashingAtIndex,GetTotalSlashing, andSetTotalSlashing) already usemath.Gweiconsistently. This aligns perfectly with the changes made to theGetSlashingsmethod and the overall PR objective.
Line range hint
1-105: Overall excellent implementation and consistency.The changes made to this file are minimal, focused, and achieve the PR objective of ensuring consistent use of
math.Gweifor slashings. The code demonstrates good practices such as:
- Consistent use of generics across methods.
- Proper error handling.
- Clear and concise documentation for each method.
The changes integrate seamlessly with the existing code, maintaining its quality and readability.
mod/node-api/handlers/proof/merkle/mock/beacon_state.go (2)
Line range hint
1-145: Overall assessment of the changesThe modification to use
math.Gweifor slashings in theNewBeaconStatefunction is consistent with the PR objective. The change is isolated to a single line and appears to be correct within the context of this file.However, it's crucial to ensure that this change is consistently applied across the entire codebase, particularly in areas that interact with slashings or expect
uint64slices for this purpose. The verification script provided in the previous comment will help identify any inconsistencies or areas that require further attention.
100-100: Verify the impact of changing slashings type tomath.GweiThe change from
[]uint64{}to[]math.Gwei{}aligns with the PR objective to make slashings usemath.Gwei. This modification enhances type consistency for slashings throughout the codebase.However, we should consider the following:
- Ensure that all other parts of the codebase expecting
uint64slices for slashings have been updated accordingly.- Verify that the
math.Gweitype has the same or better performance characteristics compared touint64for this use case.- Check if this change affects any serialization/deserialization processes, especially if the data is persisted or transmitted.
To ensure consistency across the codebase, please run the following verification script:
This script will help identify any inconsistencies or remaining work related to this change.
mod/state-transition/pkg/core/state/interfaces.go (3)
110-110: Approved: Improved type consistency for slashingsThe change from
[]uint64to[]math.Gweifor theGetSlashingsmethod return type is a positive improvement. It aligns with the PR objective of ensuringSlashingsandTotalSlashingshare the same data type, enhancing consistency within the codebase. Usingmath.Gweialso suggests a more precise representation of financial values, which is crucial for slashing calculations.
110-112: Excellent: Achieved full consistency in slashing-related methodsAfter reviewing the entire interface, it's clear that the changes to
GetSlashingsandSetSlashingAtIndexhave achieved full consistency in the representation of slashing amounts. All slashing-related methods (GetTotalSlashing,SetTotalSlashing,GetSlashings,SetSlashingAtIndex, andGetSlashingAtIndex) now uniformly usemath.Gwei. This consistency enhances the interface's clarity and reduces the potential for type-related errors in implementations.
112-112: Approved: Consistent use of math.Gwei for slashing amountsThe change of the
amountparameter type fromuint64tomath.Gweiin theSetSlashingAtIndexmethod is approved. This modification maintains consistency with theGetSlashingsmethod and ensures that slashing amounts are uniformly represented usingmath.Gweithroughout the interface.To ensure complete consistency, let's verify that all slashing-related methods in the interface use
math.Gwei:✅ Verification successful
Verified: Consistent use of math.Gwei across all slashing-related methods
All slashing-related methods in
mod/state-transition/pkg/core/state/interfaces.goconsistently usemath.Gweifor their parameters and return types. This confirms that the change toSetSlashingAtIndexmaintains uniformity throughout the interface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent use of math.Gwei in slashing-related methods # Test: Search for methods related to slashing and their types rg --type go -i '(get|set|total).*slash.*(\w+)' mod/state-transition/pkg/core/state/interfaces.goLength of output: 593
mod/consensus-types/pkg/types/state_test.go (1)
103-103: Verify test cases with the newmath.Gweitype.While the change shouldn't affect the existing tests, it's crucial to ensure they still pass with the new
math.Gweitype for theSlashingsfield.Run the following script to verify the tests:
mod/consensus-types/pkg/types/state.go (2)
72-72: LGTM: Type consistency improvementThe change from
[]uint64to[]math.Gweifor theSlashingsfield aligns well with the PR objective. This modification enhances type consistency betweenSlashingsandTotalSlashing, improving code clarity and type safety.
100-100: LGTM: Consistent method signature updateThe
slashingsparameter type in theNewmethod has been correctly updated to[]math.Gwei, maintaining consistency with theSlashingsfield type change in the struct. This ensures that the method signature aligns with the updated struct definition.mod/node-core/pkg/components/interfaces.go (3)
210-210: Type change for slashings parameterThe type of the
slashingsparameter in theNewmethod ofBeaconStateMarshallableinterface has been changed from[]uint64to[]math.U64. This change aligns with the PR objective of making slashings usemath.Gwei.
897-897: Return type change for GetSlashings methodThe return type of the
GetSlashingsmethod in theBeaconStoreinterface has been updated from[]uint64to[]math.Gwei. This change is consistent with the PR objective and the previous change in theBeaconStateMarshallableinterface.
210-210: Summary of changes: Consistent use of math.Gwei for slashingsThe changes in this file consistently update the type used for slashings from
uint64tomath.Gwei(ormath.U64in one instance). This modification aligns with the PR objective of ensuring thatSlashingsandTotalSlashingcomponents share the same data type. The changes affect the following interfaces:
BeaconStateMarshallable: Updatedslashingsparameter type in theNewmethod.BeaconStore: Updated return type of theGetSlashingsmethod.These changes enhance type consistency and improve the semantic representation of slashing values in the codebase.
Also applies to: 897-897
| @@ -54,7 +54,7 @@ type BeaconStateMarshallable[ | |||
| randaoMixes []common.Bytes32, | |||
| nextWithdrawalIndex uint64, | |||
| nextWithdrawalValidatorIndex math.U64, | |||
| slashings []uint64, totalSlashing math.U64, | |||
| slashings []math.U64, totalSlashing math.U64, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency in the type of slashings across the codebase
After reviewing the occurrences of slashings in the codebase, there is an inconsistency in the types being used:
math.U64in:mod/node-core/pkg/components/interfaces.gomod/state-transition/pkg/core/state/types.go
math.Gweiin:mod/storage/pkg/beacondb/slashing.gomod/consensus-types/pkg/types/state.go
sdkcollections.Map[uint64, uint64]in:mod/storage/pkg/beacondb/kvstore.go
This inconsistency can lead to confusion and potential type-related issues. It is recommended to standardize the type used for slashings to align with the PR objective of utilizing math.Gwei.
🔗 Analysis chain
Type change improves consistency, but differs from PR objective
The change from []uint64 to []math.U64 for the slashings parameter improves type consistency with totalSlashing. This is a positive change as it likely provides additional functionality or constraints compared to the built-in uint64.
However, there's a discrepancy between the PR objective and the actual change:
- PR objective: Make slashings use
math.Gwei - Actual change: Using
math.U64
Please clarify if math.U64 is equivalent to math.Gwei or if there's a reason for this difference. If math.Gwei should be used instead, consider updating the type accordingly.
To ensure consistency across the codebase, let's check for other occurrences of slashings and their types:
This will help verify if the type change is consistent with other parts of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of 'slashings' in Go files
rg --type go 'slashings.*\[?\]?(uint64|math\.U64|math\.Gwei)' -g '!mod/state-transition/pkg/core/state/types.go'
Length of output: 505
| @@ -100,7 +100,7 @@ func generateValidBeaconState() *types.BeaconState[ | |||
| }, | |||
| Balances: []uint64{32000000000, 31000000000}, | |||
| RandaoMixes: generateRandomBytes32(65536), | |||
| Slashings: []uint64{1000000000, 2000000000}, | |||
| Slashings: []math.Gwei{1000000000, 2000000000}, | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider updating TotalSlashing for consistency.
The change from []uint64 to []math.Gwei for the Slashings field aligns with the PR objective. This improves type safety and clarity in the code.
However, for consistency, consider updating the TotalSlashing field on line 106 to use math.Gwei as well.
Suggested change for line 106:
- TotalSlashing: 3000000000,
+ TotalSlashing: math.Gwei(3000000000),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Slashings: []math.Gwei{1000000000, 2000000000}, | |
| Slashings: []math.Gwei{1000000000, 2000000000}, | |
| TotalSlashing: math.Gwei(3000000000), |
| @@ -378,7 +378,7 @@ func (st *BeaconState[ | |||
| } | |||
| subIndx = hh.Index() | |||
| for _, i := range st.Slashings { | |||
| hh.AppendUint64(i) | |||
| hh.AppendUint64(uint64(i)) | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM with a suggestion: Consider documenting overflow risk
The casting of math.Gwei to uint64 is consistent with how TotalSlashing is handled and avoids unnecessary copying. However, it's worth considering the following:
- Document the assumption that
math.Gweiwill never exceed the range ofuint64. - Consider adding a runtime check to ensure no overflow occurs during the cast.
These steps would help prevent potential issues if math.Gwei's range ever changes in the future.
Make
SlashingsandTotalSlashingshare the same type.Summary by CodeRabbit
New Features
Bug Fixes
Refactor